Skip to content

Test for null buffer in CString.len()/.iter() and fail #11942

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

lilyball
Copy link
Contributor

Also change .as_str() to fail on null buffer.

@alexcrichton
Copy link
Member

It appears that dealing with null is a little inconsistent. Methods like len and as_str have return values, but everything else fail!s the task.

I would expect these all to do the same thing, and I would personally expect that course of action to be failure.

@lilyball
Copy link
Contributor Author

@alexcrichton Failure on .len() is the other possible course of action, but it just didn't seem particularly useful. The other methods fail because they don't have a sensible alternative behavior (well, with_ref() could yield the null pointer, but that's not very useful).

.as_str() has an optional return value because not all valid CStrings can be valid Rust strings (notably, CStrings don't require utf-8). I suppose it could return None on a non-utf8 string and fail on a null buffer, but as long as it has an optional return type already, why fail?

Incidentally, I think .iter() might also crash on null. The question is, make it fail or make it be an empty iterator? I'm tempted to make it fail. Although if I do that, that lends more credence to the suggestion to make .len() fail on null as well.

Hmm.

@lilyball
Copy link
Contributor Author

I've rewritten this to fail on null buffer. I'm also added the same failure to .iter().

Also change .as_str() to fail on null buffer.
@bors bors closed this in 5c5d995 Feb 1, 2014
@lilyball lilyball deleted the cstr-null-buf-len branch February 1, 2014 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants